feat(generator): add debug_redact to key fields and enable redact in DebugString - #16398
feat(generator): add debug_redact to key fields and enable redact in DebugString#16398colinmoy wants to merge 5 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the discovery-to-proto generator to support mapping 'any' types to google.protobuf.Value or google.protobuf.Struct instead of always defaulting to google.protobuf.Any. It also introduces automatic redaction (debug_redact = true) for string/bytes fields containing 'key' in their name and updates DebugString to respect this option. The review feedback highlights several potential nlohmann::json::type_error exceptions that could occur if JSON Schema elements (like additionalProperties, items, or format) are not objects or strings as expected, recommending explicit type checks. Additionally, it suggests refining the redaction logic with a word-boundary check to prevent false positives on fields containing 'key' as a substring (e.g., 'monkey'), utilizing std::string_view per the repository style guide.
I am having trouble creating individual review comments. Click here to see my feedback.
generator/internal/discovery_to_proto.cc (384-395)
In JSON Schema, additionalProperties can be a boolean (e.g., false or true). If items["additionalProperties"] is a boolean, calling .value("type", "") or .contains("format") on it will throw a nlohmann::json::type_error exception (specifically type_error.306 because the JSON value is not an object). We should verify that items["additionalProperties"] is an object before calling these methods.
if (IsDiscoveryArrayType(*current)) {
auto const& items = (*current)["items"];
if (items.is_object() &&
items.contains("type") && items["type"] == "object" &&
items.contains("additionalProperties") &&
items["additionalProperties"].is_object() &&
items["additionalProperties"].value("type", "") == "any" &&
!items.contains("format") &&
!items["additionalProperties"].contains("format")) {
types_to_import.insert("google.protobuf.Struct");
} else {
worklist.push_back(&items);
}
}generator/internal/discovery_to_proto.cc (397-407)
Similar to the array items check, additional_properties can be a boolean value in JSON Schema. If it is not an object, calling .contains() on it will throw a nlohmann::json::type_error exception. We should ensure additional_properties is an object before querying its properties.
if (IsDiscoveryMapType(*current)) {
auto const& additional_properties = (*current)["additionalProperties"];
if (additional_properties.is_object() &&
additional_properties.contains("type") &&
additional_properties["type"] == "any" &&
!additional_properties.contains("format") &&
!current->contains("format")) {
types_to_import.insert("google.protobuf.Struct");
} else {
worklist.push_back(&additional_properties);
}
}generator/internal/discovery_to_proto.cc (363-372)
When checking the "format" field, we should ensure it is actually a string before assigning it to std::string const format. If "format" is present but is not a string (e.g., a boolean or an integer), the implicit conversion to std::string will throw a nlohmann::json::type_error exception.
if (current->contains("format") && (*current)["format"].is_string()) {
std::string const format = (*current)["format"];
if (absl::StartsWith(format, "google.protobuf.")) {
types_to_import.insert(format);
// This node is resolved as a protobuf message (e.g. google.protobuf.Any
// on Status.details items), so do not traverse into its internal
// additionalProperties or properties.
continue;
}generator/internal/discovery_type_vertex.cc (48-56)
If field_json["items"] is not an object, calling .value("type", "") on it will throw a nlohmann::json::type_error exception. We should ensure field_json["items"] is an object before attempting to retrieve its "type".
bool IsStringOrBytes(nlohmann::json const& field_json) {
std::string const type = field_json.value("type", "");
if (type == "string" || type == "bytes") return true;
if (type == "array" && field_json.contains("items") &&
field_json["items"].is_object()) {
std::string const item_type = field_json["items"].value("type", "");
if (item_type == "string" || item_type == "bytes") return true;
}
return false;
}generator/internal/discovery_type_vertex.cc (124-132)
Ensure that v["format"] is actually a string before assigning it to type to prevent a potential nlohmann::json::type_error exception.
if (type == "any") {
if (v.contains("format") && v["format"].is_string()) {
type = v["format"];
} else {
type = "google.protobuf.Value";
}
return TypeInfo{type, compare_package_name, properties_for_synthesis, false,
false};
}generator/internal/discovery_type_vertex.cc (165-174)
Ensure that additional_properties and v are objects and their "format" fields are strings before assigning them to map_type to prevent a potential nlohmann::json::type_error exception.
} else if (map_type == "any") {
if (additional_properties.is_object() &&
additional_properties.contains("format") &&
additional_properties["format"].is_string()) {
map_type = additional_properties["format"];
} else if (v.contains("format") && v["format"].is_string()) {
map_type = v["format"];
} else {
map_type = "google.protobuf.Struct";
}
return TypeInfo{map_type, compare_package_name,
properties_for_synthesis, true, is_message};generator/internal/discovery_type_vertex.cc (208-228)
Ensure that items and items["additionalProperties"] are objects and their "format" fields are strings before querying them or assigning them to type to prevent potential nlohmann::json::type_error exceptions.
} else if (type == "any") {
if (items.is_object() && items.contains("format") &&
items["format"].is_string()) {
type = items["format"];
} else {
type = "google.protobuf.Value";
}
return TypeInfo{type, compare_package_name, nullptr, false, false};
} else if (type == "object" && items.is_object() &&
items.contains("properties")) {
// Synthesize a nested type for this array.
type = CapitalizeFirstLetter(field_name + "Item");
return TypeInfo{type, compare_package_name, &items, false, true};
} else if (type == "object" && items.is_object() &&
items.contains("additionalProperties") &&
items["additionalProperties"].is_object() &&
items["additionalProperties"].value("type", "") == "any") {
if (items.contains("format") && items["format"].is_string()) {
type = items["format"];
} else if (items["additionalProperties"].contains("format") &&
items["additionalProperties"]["format"].is_string()) {
type = items["additionalProperties"]["format"];
} else {
type = "google.protobuf.Struct";
}
return TypeInfo{type, compare_package_name, nullptr, false, false};generator/internal/discovery_type_vertex.cc (510-514)
The current substring check absl::StrContains(field_name, "key") is highly prone to false positives and will over-redact fields that happen to contain "key" as a substring but are not actual keys (e.g., monkey, keyboard, hockey, keypad, keynote).
Since field_name is already formatted as snake_case by the generator, we can perform a precise word-boundary check to ensure "key" is matched only as a standalone word (e.g., kms_key_name, raw_key, rsa_encrypted_key, or key itself). This also eliminates the need to check json_field_name separately.
You can add a helper function ContainsKeyWord in the anonymous namespace:
bool ContainsKeyWord(std::string_view s) {
for (std::size_t pos = s.find("key"); pos != std::string_view::npos;
pos = s.find("key", pos + 1)) {
bool const prefix_ok = (pos == 0 || s[pos - 1] == '_');
bool const suffix_ok = (pos + 3 == s.size() || s[pos + 3] == '_');
if (prefix_ok && suffix_ok) return true;
}
return false;
} if (IsStringOrBytes(field_json) && ContainsKeyWord(field_name)) {
field_options.emplace_back("debug_redact", "true");
}References
- Prefer using
std::string_viewinstead ofabsl::string_viewin C++ code. (link)
… and JSON type safety
| } | ||
|
|
||
| if (current->contains("format")) { | ||
| if (current->contains("format") && (*current)["format"].is_string()) { |
There was a problem hiding this comment.
If the discovery document contains a format field that is not a string, this is a fundamental problem that the generator cannot recover from. We should emit an error and exit.
| auto const& items = (*current)["items"]; | ||
| if (items.contains("type") && items["type"] == "object" && | ||
| items.contains("additionalProperties") && | ||
| if (items.is_object() && items.contains("type") && |
There was a problem hiding this comment.
Similarly, if this isn't an object, then the discovery document is invalid. Emit an error and exit.
Same below. If checks to verify the structure of the document fail, these are non-recoverable.
| return false; | ||
| } | ||
|
|
||
| bool ContainsKeyWord(std::string_view s) { |
There was a problem hiding this comment.
Prefer using string matching functions from absl (e.g. StrContains) and the absl Suffix/Prefix functions instead of std::string::find and iterators directly.
…n across protobuf versions
2 participants
Fixes: b/510919426